Feat/reset view on hyperspace exit #5922

mwerle · 2024-09-27T01:19:19Z

Background

The current game has a concept of a "selected system" in the Sector Map. Whenever opening the System Overview or the System Map, this is the system whose details will be displayed.

The only way to change the "selected system" is by the user changing it in the Sector Map.

This is somewhat unintuitive from a players' perspective. When the player is interacting with the various maps, then yes, this is the expected behaviour, however, when the player is jumping into a new system, upon arrival, it is more intuitive for the System Map and System Overview to show the current system.

Proposed changes

This PR proposes to fix this with 2 partially-related changes:

Add an option to reset the view to the WorldView following a hyperspace jump.
Add a selection button to the Map Views allowing the player to choose to display either the current system, or the selected system.

When the option to reset the view to the WorldView is unchecked (default), and the Map Display is set to the selected system (default), there is no change to the game behaviour from the current behaviour.

Reset View after hyperspace exit

This was originally added to avoid certain issues with the selected system changing, but is kept as some players may prefer this . This could be split into a separate PR.

A benefit of enabling this option is that players can immediately react to hostile actions following a hyperspace exit.

An improvement to this option may be to also reset the view to "internal" and "front" - the current implementation maintains the last camera setting.

Add Map View mode to selected or current

When the Map View is set to the current system, the System Map and the System Overview will show the current system instead of the selected system. This can be especially useful if the player is exploring and wishes to quickly examine the current system while following a hyperspace route of multiple jumps.

One side-effect of this change is that if the player is currently viewing one of the maps while performing a hyperspace jump, then the view is updated as soon as the new system is reached.

On the one hand this could be seen as a benefit as the player is immediately notified of arrival in a new system, on the other it could be disruptive if the player is currently examining some aspect of the previous system.

It would require additional work, potentially complex and fragile, to keep the view on the currently opened system.

Issues with these changes

Apart from those already mentioned, there are no known issues or new bugs introduced by these changes.

Original Draft-PR Description

## Background The current game has a concept of a "selected system" in the `Sector Map`. Whenever opening the `System Overview` or the `System Map`, this is the system whose details will be displayed.

The only way to change the "selected system" is by the user changing it in the Sector Map.

This is somewhat unintuitive from a players' perspective. When the player is interacting with the various maps, then yes, this is the expected behaviour, however, when the player is jumping into a new system, upon arrival, it is more intuitive for the System Map and System Overview to show the current system.

Proposed changes

This PR proposes to fix this by resetting the "selected system" to the "current system" when entering a new system.

Issues with the fix

The current implementation has the downside that if the player is viewing the System Map or System Overview while performing a hyperspace jump, the map is changed as soon as the jump is complete, even if the player is interacting with the map at the time.

Discussion

Hence this PR is still a "draft" while ways to prevent the map from changing are investigated. While some players might like the current approach (ie, it's obvious when the jump is complete), others might prefer to keep viewing the old map.

One solution could be that the player is forcibly returned to the flight screen when the hyperspace jump is completed. This has the benefit that the player is immediately able to react to, for example, hostile actions by other ships.
This should be fairly easy to implement.

Another suggestion has been that the "selected system" is not changed, but rather that the System Map and System Overview show the "selected system" when opened from the Sector Map (and possibly some other screens such as the BBS), but the "current system" when opened from the spaceflight screen. This might lead to confusing behaviour depending on the exact sequence of opening maps and the starting screen compared to what the player expects. This is also likely to be somewhat more complex to implement.

impaktor · 2024-09-27T05:39:36Z

The current implementation has the downside that if the player is viewing the System Map or System Overview while performing a hyperspace jump, the map is changed as soon as the jump is complete

I suspect this is an edge use case. I think we had a crash bug when doing this, and took a while before it was discovered.

sturnclaw

This looks good to me, thank you for tackling it! If no one else volunteers to test first, I'll give it a playtest early next week once my worktree is clean again and merge.

sturnclaw · 2024-09-27T19:04:33Z

This PR can also of course be extended to tackle some of the issues with the proposed change and I'll defer merge until that's been accomplished at your discretion.

I similarly had the idea that it might be good to have an explicit "view system" button in the Sector Map which opened the System View on that specific system and otherwise decouple the System View from the Sector Map selection.

mwerle · 2024-09-29T13:21:27Z

So I did some testing and it seems that moving the code to LUA breaks it - calling ResetHyperspaceTarget in LUA breaks a route as it doesn't advance the hyperspace target to the next system in the route. No idea why it works when called from the C++ but breaks when called from LUA. Possibly because the LUA runs after the C++, and the order is now changed? In either case, not explicitly calling ResetHyperspaceTarget at all seems to also work.. looking at the LUA, it appears that it is called anyway as part of hyperJumpPlanner.onEnterSystem from the ship.onEnterSystem callback.

I've also added code to force-change the view back to the game view when exiting Hyperspace, and I think that might not be a bad option to add to the game. Not sure how to add options though...

(offtopic)
I find it a royal PITA to try to reverse-engineer the logic flow when half the logic is in LUA instead of C++.. C++ I can set breakpoints and trace through, LUA is a black box. Personally I am finding contributing to this mish-mash of C++ and LUA to be quite a challenge. I know it was mentioned that moving logic to LUA is to make it easier to contribute, but.. well, definitely not for me :)

(offtopic 2)
How do you use the LUA console? It seems the wiki page is extremely outdated, and I can't figure out how to add some commodities (hydrogen) to the ship in the current version.. that would make testing this feature a lot easier..

I got as far as require "CargoManager".AddCommodity()" but it wants an actual type for the commodity, not a string. I managed to retrieve a "CommodityType" for "hydrogen" as well, but passing it to AddCommodity just gives me errors; something about trying to index a value (number). At this point I gave up.

mwerle · 2024-09-29T13:48:22Z

Not sure how to add options though...

NVM, figured it out.

mwerle · 2024-09-29T14:36:37Z

Ok, so I've added an option "Reset View on Hyperspace Exit". This option is on a new tab "Options", and I've also moved the "Enable Autosave" option to this tab as it's got nothing to do with the various graphics settings. This is done in 2 commits to logically separate the work.

[EDIT: just saw that a game-settings tab was also implemented in #5655]

If "Reset View on Hyperspace Exit" is unchecked, then the game stays on whichever view the player is currently in. If the current view is the "System Map" or the "System Overview", this will change to the current system when the hyperspace jump is completed.

If "Reset View on Hyperspace Exit" is checked, the player is ejected back to the Game View from whichever screen they are currently in.

Brief testing appears to show no issues.

If people like these changes, I will squash the various commits and convert the PR from draft to a real PR. Otherwise open to improvement suggestions.

mwerle · 2024-09-29T14:51:12Z

I similarly had the idea that it might be good to have an explicit "view system" button in the Sector Map which opened the System View on that specific system and otherwise decouple the System View from the Sector Map selection.

I'm not entirely sure I follow this thought. Which system should be opened from this explicit "View System" button? The only one that makes sense is the currently selected system.. which is what happens already. Unless you add a right-click menu for a star..

impaktor · 2024-09-29T17:59:10Z

@mwerle I think you might want to try the debug tool we have in-game: Ctrl+i to give yourself hydrogen, ship, enemy, etc.
As for lua console, I believe it's bound to the key you've set in settings.

mwerle · 2024-09-29T23:07:08Z

I think you might want to try the debug tool we have in-game

Thank you; that is very useful! I had actually stumbled on this before, but didn't see the section where you can buy commodities through it.

As for lua console, I believe it's bound to the key you've set in settings.

Well, yes, I can open it.. but the example given on the wiki page to add cargo doesn't work (I didn't try the others) and I couldn't figure out a way to make it work. No longer required due to the debug tool, but we might want to update the wiki..

mwerle · 2024-09-30T10:01:22Z

Ok, big changes to the proposal.

No longer change the currently selected system on hyperspace exit.

-> Reverted in commit 7f7f701

Instead, add a new button to the System Map and System Overview screens to switch the display between "Current" system and "Selected" system (defaults to "Selected" to keep current mode of operation). If the display has been switched to "Current" system, then the map is updated on hyperspace exit to show the arrival system.

-> Implemented in Commit 6c8d551

Also add a new game setting to "Reset View on Hyperspace Exit". When checked, the view is reset to the game view when exiting hyperspace. The view itself is not changed (ie, internal/external/direction). There's an argument to reset the view to internal-forward..

-> Implemented in commits c666827, c666827, c666827, and c666827

Also found and fixed an edge-case bug where the next hyperspace system was not being properly updated on hyperspace exit if the player did not enter the "Sector Map" following starting and loading the game, and immediately starting a hyperspace jump.

Fixed in d740e42

Also refactored the "edge buttons" of all map screens - they are now aligned to the top-right, and the first set of buttons is the same between all screens.

-> Changed in commit c666827

Feedback welcome; especially choice of icons. Have done some testing, but not comprehensive yet (will do so after dinner).

Test plan:

Game View:

Jump while in game-view with "reset view" unchecked
EXPECT: jump completes, no change to views or systems
ACTUAL:
Jump while in game-view with "reset view" checked
EXPECT: jump completes, no change to views or systems
ACTUAL:

No-Game and Non-Map View (eg, Personal Details):

Jump while in a non-map view with "reset view" unchecked
EXPECT: jump completes, view doesn't change
ACTUAL:
Jump while in a non-map view with "reset view" checked
EXPECT: jump completes, view returns to game-view; game view should be same as before
ACTUAL:

Sector Map View:

Jump while in Sector Map view with "reset view" unchecked
EXPECT: jump completes, map is updated with Current System (and possibly Hyperspace Target) updated
ACTUAL:
Jump while in Sector Map view with "reset view" checked
EXPECT: jump completes, view returns to game-view; game view should be same as before
ACTUAL:

System Map or System Overview:

Jump while in view with "reset view" unchecked and "system selection" is set to "selected"
EXPECT: jump completes, no change to view
ACTUAL:
Jump while in view with "reset view" unchecked and "system selection" is set to "current"
EXPECT: jump completes, map changes to arrival system
ACTUAL:
Jump while in a Sector Map view with "reset view" checked
EXPECT: jump completes, view returns to game-view; game view should be same as before
ACTUAL:

mwerle · 2024-09-30T11:28:58Z

Test Run : Micha 2024-09-30

Game View:

Jump while in game-view with "reset view" unchecked
- EXPECT: jump completes, no change to views or systems
- ACTUAL:
  - Jump: completed successfully
  - View: unchanged
  - Selected system: unchanged
  - Hyperspace target: moved to next system in route
Jump while in game-view with "reset view" checked
- EXPECT: jump completes, no change to views or systems
- ACTUAL:
  - Jump: completed successfully
  - View: unchanged
  - Selected system: unchanged
  - Hyperspace target: moved to next system in route

No-Game and Non-Map View (eg, Personal Details):

Jump while in a non-map view with "reset view" unchecked
- EXPECT: jump completes, view doesn't change
- ACTUAL:
  - Jump: completed successfully
  - View: unchanged (log view: new entry added on arrival)
  - Selected system: unchanged
  - Hyperspace target: moved to next system in route
Jump while in a non-map view with "reset view" checked
- EXPECT: jump completes, view returns to game-view; game view should be same as before
- ACTUAL:
  - Jump: completed successfully
  - View: returned to game-view
  - Selected system: unchanged
  - Hyperspace target: moved to next system in route

Sector Map View:

Jump while in Sector Map view with "reset view" unchecked
- EXPECT: jump completes, map is updated with Current System (and possibly Hyperspace Target) updated
- ACTUAL:
  - Jump: completed successfully
  - View: remains on Sector Map; animated to show new current system and hyperspace target system
  - Selected system: unchanged
  - Hyperspace target: moved to next system in route
Jump while in Sector Map view with "reset view" checked
- EXPECT: jump completes, view returns to game-view; game view should be same as before
- ACTUAL:
  - Jump: completed successfully
  - View: remains on Sector Map; animated to show new current system and hyperspace target system; then returns to Game View (same view as before; front, left, right, etc)
  - Selected system: unchanged
  - Hyperspace target: moved to next system in route

System Map or System Overview:

Jump while in view with "reset view" unchecked and "system selection" is set to "selected"
- EXPECT: jump completes, no change to view
- ACTUAL:
  - Jump: completed successfully
  - View: remains on System Map, displayed system unchanged
  - Selected system: unchanged
  - Hyperspace target: moved to next system in route
Jump while in view with "reset view" unchecked and "system selection" is set to "current"
- EXPECT: jump completes, map changes to arrival system
- ACTUAL:
  - Jump: completed successfully
  - View: remains on System Map, displayed system changed to new system
  - Selected system: unchanged
  - Hyperspace target: moved to next system in route
Jump while in a Sector Map view with "reset view" checked
- EXPECT: jump completes, view returns to game-view; game view should be same as before
- ACTUAL:
  - Jump: completed successfully
  - View: returned to Game View; in-flight view is same as before
  - Selected system: unchanged
  - Hyperspace target: moved to next system in route

Test savefile

(Remove the ".zip" ending)

Hyperspace_test.zip

impaktor · 2024-09-30T11:33:03Z

regarding pain of lua/c++ development:

there's ability for hot reloading the lua files, so you don't need to restart pioneer between each change
there's ability to send lua code from your IDE to running pioneer instance.

mwerle · 2024-09-30T11:44:44Z

regarding pain of lua/c++ development:

* there's ability for hot reloading the lua files, so you don't need to restart pioneer between each change

* there's ability to send lua code from your IDE to running pioneer instance.

It's mostly not being able to trace through to see the logic in the Lua executing with variable views. It took me well over 3 hours to track down a rather trivial bug, which should have taken less than 10 minutes. And I only cracked it because I went back to the "poor man's debugger" and added printf's everywhere :P Oh well, it is what it is. (In Japanese: shoganai; wonderful word!)

But yes, the hot-reload is very nice when fiddling with UI layouts. And, I suspect, tweaking of AI routines for the NPC ships. Personally I'd still leave the vast majority of the game logic in the C++ though.. just add relevant Lua hooks everywhere for modders.

sturnclaw · 2024-10-01T18:18:59Z

I find it a royal PITA to try to reverse-engineer the logic flow when half the logic is in LUA instead of C++.. C++ I can set breakpoints and trace through, LUA is a black box.

This is definitely a pain-point to be sure. We have a function void pi_lua_stacktrace(lua_State *l) that you can execute inside a C++ side breakpoint/assert to print the Lua callstack, and a very verbose set of context information is written to the output.txt log when Lua encounters an error, but a general-purpose Lua debugger is something we're definitely missing. It's not the easiest thing to implement either, though there might be an existing project we can leverage in this regard for IDE compatibility.

How do you use the LUA console? It seems the wiki page is extremely outdated, and I can't figure out how to add some commodities (hydrogen) to the ship in the current version.. that would make testing this feature a lot easier..

The wiki is quite outdated and the developer-facing portions are in the process of being moved to https://dev.pioneerspacesim.net/. We have a debug tool for cargo as you've found, so updating that wiki has been historically low-priority. If you want to programmatically add cargo you can do so via:

player:GetComponent('CargoManager'):AddCommodity(require 'CommodityType'.GetCommodity("hydrogen"), 2)

Note that we also have a Lua codedoc providing documentation for the various Lua utilities and classes.

If you're using VSCode or another language-server-protocol compatible IDE, the LuaLS tool is incredibly helpful and provides type information and autocomplete when working with type-annotated Lua files. We're incrementally working on expanding the type information / annotation coverage of the Lua codebase as time goes by.

sturnclaw · 2024-10-01T18:29:15Z

Ok, so I've added an option "Reset View on Hyperspace Exit". This option is on a new tab "Options", and I've also moved the "Enable Autosave" option to this tab as it's got nothing to do with the various graphics settings. This is done in 2 commits to logically separate the work.

This sounds good. I'd name the tab "Game Options" specifically, as it has to do with in-game behaviors.

If "Reset View on Hyperspace Exit" is checked, the player is ejected back to the Game View from whichever screen they are currently in.

I'd recommend renaming this to "Close Map on Hyperspace Exit" - "Reset View" is semantically overloaded and can refer to changing the camera mode, changing the camera orientation, or changing to the WorldView.

If people like these changes, I will squash the various commits and convert the PR from draft to a real PR. Otherwise open to improvement suggestions.

I like this change, thank you!

I'm not entirely sure I follow this thought. Which system should be opened from this explicit "View System" button? The only one that makes sense is the currently selected system.. which is what happens already. Unless you add a right-click menu for a star..

The idea here was to completely remove the automatic system selection of the system map, and instead add an explicit button in the sector map to open the system view for the selected system, which system would remain open in the system map even if the sector map cursor was moved.

Because the default behavior would be for the system view to not change the system viewed unless the user explicitly commanded the viewing of a different system, we'd most likely need a separate button accessible from the WorldView to open the current system in the system map.

This is a quite involved change however, so I'm merely proposing it for discussion / evaluation rather than implementation.

Note that you can collapse long lists on Github inside a <details> tag like so - I've gone ahead and done so for the the testing lists above to make the discussion easier to read.

<details>

The empty lines are important
Some long list

</details>

sturnclaw · 2024-10-01T18:51:47Z

Also refactored the "edge buttons" of all map screens - they are now aligned to the top-right, and the first set of buttons is the same between all screens.

Note that these edge buttons are loosely intended to be converted to use the sidebar.lua widget used on the main WorldView screen, similar to this mockup. The map views are pending a UI design/normalization pass which may involve some aggressive redesigning.

Personally I'd still leave the vast majority of the game logic in the C++ though.. just add relevant Lua hooks everywhere for modders.

There is a lot of opinionated game logic in Pioneer that is neither a performance concern nor a "universal constant" but is still hard-coded in C++ and cannot be altered without significant work.

My personal approach is that logic in C++ should be "composable" - it's a set of building blocks that the "default mod" arranges to produce the intended balance and behavior of the game. Other mods can then override and alter the arrangement of those building blocks to produce a different result.

What we have now is the "sprinkle hooks everywhere" method, and it's produced a very brittle and inflexible codebase. For example, fuel scooping is "game logic" that is (and was even more so) completely hardcoded in C++. As a result, mods cannot e.g. allow scooping a star, change the minimum atmospheric density to scoop, or allow scooping any other material other than thruster fuel.

...that's a bit of an off-topic rant though. I do agree that a lot of game logic can (and often should) live in C++, but there has to be a great amount of attention paid to how that logic interconnects with other parts of the game.

All that out of the way, I'll try to give this branch a playtest soon and provide more actionable feedback about the concerns you raised above. My mental queue is currently full, so it may be a few days however.

mwerle · 2024-10-01T23:59:28Z

I'd name the tab "Game Options"

Question

Do you mean the internal name of the tab, or the tooltip text (or both)? I simply chose "Options" for the tooltip to re-use an existing translation but trivial to update.

I'd recommend renaming this to "Close Map on Hyperspace Exit" - "Reset View" is semantically overloaded and can refer to changing the camera mode, changing the camera orientation, or changing to the WorldView.

Well, it does the last - returns to the Game View (is that "WorldView"? I am meaning the in-flight view, either internal or external) irrespective of what the current view is. It doesn't just return to the Game View from the various map screens, but also from the various informations screens which can be accessed in-flight.

There's an argument for not only returning to the Game View but also resetting the view to "internal-front" so the player can instantly react to hostile actions etc. For now I did not change the actual view configuration as the player might prefer to play in external-camera mode.

The idea here was to completely remove the automatic system selection of the system map, and instead add an explicit button in the sector map to open the system view for the selected system, which system would remain open in the system map even if the sector map cursor was moved.

So the user can already toggle the automatic system selection in the Sector Map; if the user doesn't like this, they can always disable that, which means the System Map / System Overview remain on the currently selected system.

And with my addition of toggling between the currently selected system and the current system there's no need for the user to change the currently selected system unless they want to view a different system.

So while it doesn't quite work the way you envision with explicit buttons and a second "selected map system" state, the user-story that you envision can, I think, be achieved with the currently proposed changes.

The map views are pending a UI design/normalization pass which may involve some aggressive redesigning.

I can revert the edge-button changes; it just made a little bit more sense to me to have them in a more fixed location and with a more consistent ordering.

If no coding has actually been starting on the proposed UI update, then I don't think my changes would interfere with that work and, until that is done, it is a slightly improvement IMHO.

EDIT
As initial feedback, personally I feel that mockup is condensing too much information into a single side-view, which the user needs to manage by opening/collapsing various items in order to see all the information. This would be especially true on lower screen resolutions. Is there a reason the right-side-bar isn't used for some of this information? Most monitors these days are significantly wider than tall..

Also it would require opening the side-view before being able to use the various action buttons. Some of these buttons only toggle additional information views, but some perform actual actions. I think it makes sense to condense the various information views into a sidebar, but not so sure about the action buttons.
/EDIT

Question

I can leave the edge-button refactor as-is for now or revert, please let me know your preference.

Thank you for your time in replying and evaluating this PR, as well as your additional information about the game design and debugging/investigating the Lua components while developing. I will try your suggestions.

impaktor · 2024-10-02T08:35:18Z

returns to the Game View (is that "WorldView"? I am meaning the in-flight view

Yes, that's the WorldView
Then we have SectorView and SystemView
(We used to have GalaxyView as well, that could be resurrected, if anyone is interested. Didn't serve a function, other than immersion)

I must say, it's really great to see some more contributors playing with our source code. Also @Web-eWorks is currently swamped with #5734 (feel free to help playtest and/or review, although the latter would be daunting for any mortal man), so replies & review of your PRs might be a while.

As initial feedback, personally I feel that mockup is condensing too much information into a single side-view, which the user needs to manage by opening/collapsing various items in order to see all the information. This would be especially true on lower screen resolutions. Is there a reason the right-side-bar isn't used for some of this information? Most monitors these days are significantly wider than tall. Also it would require opening the side-view before being able to use the various action buttons. Some of these buttons only toggle additional information views, but some perform actual actions. I think it makes sense to condense the various information views into a sidebar, but not so sure about the action buttons.

I'll let @bszlrd answer ☝️

sturnclaw · 2024-10-02T18:40:04Z

Do you mean the internal name of the tab, or the tooltip text (or both)?

The tooltip text specifically shown to the user, as well as an appropriate icon. Internal name can be whatever is semantically connected to the text that is shown to the user - either is fine.

Well, it does the last - returns to the Game View

I saw that it closes e.g. the Personal Info screen as well on exiting hyperspace after writing that comment. Leaving the name of the option as-is is fine in light of that information - a clearer name would be too verbose.

And with my addition of toggling between the currently selected system and the current system there's no need for the user to change the currently selected system unless they want to view a different system.

This is good, thanks! I like what you're describing. I expect this to be ergonomic enough that there's no need for my proposed solution.

I feel that mockup is condensing too much information into a single side-view [...] Is there a reason the right-side-bar isn't used for some of this information? Most monitors these days are significantly wider than tall..

Yes - it's a mockup to show general layout and hierarchy, not final layout and design. 😄

My intention when implementing that mockup would be to split functionality between left and right sidebars as needed, moving commonly-used context-free buttons (i.e. rotate map, reset view) either to a central button dock at the top/bottom center of the screen, or simply as a "regular button" in the row of sidebar toggle buttons.

I can leave the edge-button refactor as-is for now or revert, please let me know your preference.

I've not yet gotten a chance to play with it or review the code, but I'll say leave as-is. This was mostly to inform you about future plans for the screen so you don't feel like your work is accepted and then (eventually) immediately discarded in a redesign.

Thank you for your time in replying and evaluating this PR

And thank you for your time developing it! I'm happy to spend the time I have available to accelerate other contributors - many hands make light work, after all. I don't know if I can promise a timeline for "final review" but as soon as I clear some other IRL responsibilities off my plate I'll try to give this a formal review + playtest.

mwerle · 2024-10-03T02:14:37Z

The tooltip text specifically shown to the user, as well as an appropriate icon. Internal name can be whatever is semantically connected to the text that is shown to the user - either is fine.

Fixed. The tooltip is now "Game Options". I left the icon as the "cog" as that is a very commonly used icon for settings/options. If there is a better icon to use, please suggest one.

I don't know if I can promise a timeline for "final review"

Of course not; this is a hobby and I see you have a major feature implementation which has just entered review as well. There is no expectation for this PR to be reviewed within a specific timeframe.

Rebased/squashed commits and reworded the PR. Based on the initial feedback, I think this PR is now ready for proper review.

impaktor · 2024-10-03T09:07:42Z

data/lang/ui-core/de.json

@@ -715,6 +715,10 @@
    "description": "",
    "message": "Vollbildmodus"
  },
+  "GAME_OPTIONS": {


You only need to update the en.json file, it's the "master", this will then propagate new strings to all other language files (might be temporarily not working though, but that's on me).

As I speak German, I added the German translation as well..

Your edit will probably get over-written by transifex. I can never quite remember what happens when editing the non-en.json files.

If you want to maintain the german translation, that should be done through transifex.

Removed the german translations.. will add them again when they become available on transifex.

That said, I think it would be better if people could supply translations however they wished, with transifex being one option. transifex should populate itself from the existing data.. but no idea if that actually is possible. First time hearing about that system.

sturnclaw

I'm quite happy with this! Two minor nitpicks in terms of consistency and maintainability, but overall I like the effect of the changes.

Something to consider for a future PR - the player might want a "shortcut" to open the system view and switch to current system view mode from the WorldView (and similarly for selected system mode from the Sector Map) - it's extra cognitive load to check that the map is in the correct view mode after opening it.

That's not something I expect you to implement at current however; it's a better topic for later down the road when the Sector Map UI gets looked at for a rework.

data/lang/ui-core/en.json

src/enum_table.cpp

sturnclaw · 2024-10-05T20:16:51Z

src/SystemView.h

+	enum class SystemSelectionMode {
+		CurrentSystem = 0,
+		SelectedSystem = 1,
+	};


This enum should be annotated for scan_enums.py to generate enum_table entries.

Suggested change

enum class SystemSelectionMode {

CurrentSystem = 0,

SelectedSystem = 1,

};

enum class SystemSelectionMode { // <enum name=SystemSelectionMode scope='SystemView::SystemSelectionMode' public>

CurrentSystem = 0,

SelectedSystem = 1,

};

Fixed in 62aad68

Generated code should (IMHO) never be checked into source control; instead, the relevant generator should be invoked during configuration and/or build time..(*) However, requiring this would mean developers would need a version of Python2 installed.. which is currently not the case as long as a developer doesn't add a new enum which should be usable by Lua.

That said, if I'd read the top of the file I guess I would have figured it out too ;) I literally only searched for the other enum and inserted all the required bits for my new one.

(*) Which, it appears, does happen on system which have a version of Python 2 in the path as python - on modern Debian systems, this is not the case. I fixed this in 5bcbfd6

So there's one specific problem with this approach - using the Ninja generator, every time a rebuild or incremental compile is triggered every file which includes enum_tables.h is recompiled as the file has "changed" on disk. Worse yet, it looks like those changes to enum_tables.h aren't actually picked up the first time the project is built following a valid change to an annotated enum - it seems CMake (or in this case Ninja) scans dependencies before invoking the scan_enums.py script, so it's actually the second rebuild that produces a valid program state.

I'd ask that you remove the dependency between pioneer-core and the scan_enums invocation - instead, leave it defined as a CMake command and we can document it as something that should be run manually in the (very low-frequency) case of modifying an annotated enum value.

While I do agree with you in the general case that autogenerated code should likely not be checked into the build system at all, this is a sort of "gray area" where the file typically changes only once every few months, does not have easily-tracked dependencies to determine when it should be re-generated, and touches several distinct areas of the codebase (for example, the script also generates data/meta/Constants.lua). As a result it's (historically, in this codebase) "easier" for it to be checked into source control and manually re-generated when actually necessary.

I couldn't figure out a way of running it manually and assumed that it just ran at some point during the build process, hence adding the explicit dependency to ensure it ran early in the build.

ninja enums gave me an invalid target warning. I'll investigate why.

Good catch that it doesn't build properly the first time around.

Perhaps instead of having it in the CMake just document that it needs to be invoked? Or run it as part of the build configuration phase?

...and trying again now it works as expected, with both ninja and make invoking the command.. so removing the target dependency, but leaving in the change to properly find Python 2.

Thanks! Having it in CMake is mostly so it can be quickly invoked, e.g. via cmake --build build/ --target enums or make -C build enums, as well as serving as "living documentation" for the correct invocation of the script.

mwerle · 2024-10-06T13:47:31Z

Rebased on master and added requested changes. Will squash and force-push once the review is accepted.

sturnclaw

Pending the change to the scan_enums CMake target, I give this PR my blessing. Thank you @mwerle for the idea and implementation!

The "scan_enums.py" script requires a Python v2 interpreter to be available. The "CMakeLists.txt" simply searched for a Python interpreter, which failed on modern Debian systems. Instead of simply searching for "python", the CMake script was adjusted to specifically search for a Python 2 interpreter, and then to explicitly invoke that to run the "scan_enums.py" scripts.

Add a new Options tab to the settings dialog for "Game Options" in preparation for adding additional settings to the game. Move the "Autosave" setting to this new tab. This option is currently in the "Graphics Options" tab where it does not belong.

Add an option to reset the view to the WorldView on hyperspace exit to the "Game Options" tab of the settings window. When this option is enabled, the view is reset to the WorldView after a hyperspace jump.

Prepare for additional buttons to be added to the "edge buttons" area of the map views. Move the edge buttons to the top-right of the display, so all icons remain in a fixed location irrespective of how many buttons there are. This makes it easier to learn muscle-memory. Also, rearrange the icons so that all 3 map views have the same list of buttons from the top down, with any map-specific buttons following the "Info" button. Finally, remove the superfluous edge button to switch between System Map and System Overview - they already exist on the top button bar.

Add a button to switch between the Current System and the Selected System in the System Map and the System Overview screens. With this change, if the player has selected to view the current system and is viewing one of the maps during a hyperspace jump, a situation can occur in which SystemView::CalculateStarportHeight() is called while the game state is in hyperspace. Hence a check is required that the game is in normal space prior to calculating the real height of a starport above a planet surface.

Minor improvement to prevent unnecessary "onHyperspaceTargetChanged" events from being generated if the hyperspace system has not actually been changed to a new system.

The call to ResetHyperspaceTarget in Player::OnEnterSystem has been superfluous since probably commit 1e2e471. As there was a comment that the SectorView should not be invoked from this callback, and recent testing showed that it did not impact the game logic, lets remove it and resolve the comment.

mwerle · 2024-10-07T22:59:57Z

Remove target dependency for the enums target and rebased on current master.

Thank you for reviewing and the various comments, suggestions, and assistance with implementing these changes.

impaktor · 2024-10-08T08:22:30Z

Perhaps instead of having it in the CMake just document that it needs to be invoked? Or run it as part of the build configuration phase?

If there's anything missing from COMPILING.txt that can be helpful for package maintainers, or anyone wanting to compile & run pioneer, please add.

mwerle mentioned this pull request Sep 27, 2024

System Map is not updated after hyperjump #5691

Closed

mwerle force-pushed the feat/reset_selected_system branch from 2155638 to c388417 Compare September 27, 2024 01:22

bszlrd added the Cosmetic or UI label Sep 27, 2024

sturnclaw approved these changes Sep 27, 2024

View reviewed changes

mwerle force-pushed the feat/reset_selected_system branch from c388417 to fb745c4 Compare September 29, 2024 14:31

mwerle force-pushed the feat/reset_selected_system branch from 7f7f701 to 34adbe5 Compare October 3, 2024 02:10

mwerle marked this pull request as ready for review October 3, 2024 02:11

mwerle changed the title ~~Feat/reset selected system~~ Feat/reset view on hyperspace exit Oct 3, 2024

sturnclaw self-requested a review October 3, 2024 02:21

impaktor reviewed Oct 3, 2024

View reviewed changes

sturnclaw requested changes Oct 5, 2024

View reviewed changes

mwerle force-pushed the feat/reset_selected_system branch from 34adbe5 to 62aad68 Compare October 6, 2024 13:40

sturnclaw approved these changes Oct 7, 2024

View reviewed changes

mwerle added 7 commits October 8, 2024 07:55

refactor: create game options tab

7cf6652

Add a new Options tab to the settings dialog for "Game Options" in preparation for adding additional settings to the game. Move the "Autosave" setting to this new tab. This option is currently in the "Graphics Options" tab where it does not belong.

feat: reset view on hyperspace exit

99b0acd

Add an option to reset the view to the WorldView on hyperspace exit to the "Game Options" tab of the settings window. When this option is enabled, the view is reset to the WorldView after a hyperspace jump.

refactor: improve SectorView::SetHyperspaceTarget

5e7d486

Minor improvement to prevent unnecessary "onHyperspaceTargetChanged" events from being generated if the hyperspace system has not actually been changed to a new system.

mwerle force-pushed the feat/reset_selected_system branch from 62aad68 to 7c5a654 Compare October 7, 2024 22:58

sturnclaw merged commit 5a3724c into pioneerspacesim:master Oct 8, 2024
4 checks passed

mwerle deleted the feat/reset_selected_system branch October 8, 2024 00:17

This was referenced Oct 9, 2024

Improvement - move calls to Game.SaveGameStats into Jobs #5929

Closed

View is sometimes reset to WorldView if "Reset View On Hyperspace Exit" is enabled #5935

Closed

Feat/reset view on hyperspace exit #5922

Feat/reset view on hyperspace exit #5922

Conversation

mwerle commented Sep 27, 2024 • edited Loading

Background

Proposed changes

Reset View after hyperspace exit

Add Map View mode to selected or current

Issues with these changes

Original Draft-PR Description

Proposed changes

Issues with the fix

Discussion

impaktor commented Sep 27, 2024

sturnclaw left a comment

Choose a reason for hiding this comment

sturnclaw commented Sep 27, 2024

mwerle commented Sep 29, 2024 • edited Loading

mwerle commented Sep 29, 2024

mwerle commented Sep 29, 2024 • edited Loading

mwerle commented Sep 29, 2024

impaktor commented Sep 29, 2024

mwerle commented Sep 29, 2024

mwerle commented Sep 30, 2024 • edited by sturnclaw Loading

Test plan:

Game View:

No-Game and Non-Map View (eg, Personal Details):

Sector Map View:

System Map or System Overview:

mwerle commented Sep 30, 2024 • edited by sturnclaw Loading

Test Run : Micha 2024-09-30

Game View:

No-Game and Non-Map View (eg, Personal Details):

Sector Map View:

System Map or System Overview:

Test savefile

impaktor commented Sep 30, 2024

mwerle commented Sep 30, 2024

sturnclaw commented Oct 1, 2024

sturnclaw commented Oct 1, 2024

sturnclaw commented Oct 1, 2024

mwerle commented Oct 1, 2024 • edited Loading

impaktor commented Oct 2, 2024

sturnclaw commented Oct 2, 2024

mwerle commented Oct 3, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sturnclaw left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mwerle commented Oct 6, 2024

sturnclaw left a comment

Choose a reason for hiding this comment

mwerle commented Oct 7, 2024

impaktor commented Oct 8, 2024

mwerle commented Sep 27, 2024 •

edited

Loading

mwerle commented Sep 29, 2024 •

edited

Loading

mwerle commented Sep 29, 2024 •

edited

Loading

mwerle commented Sep 30, 2024 •

edited by sturnclaw

Loading

mwerle commented Sep 30, 2024 •

edited by sturnclaw

Loading

mwerle commented Oct 1, 2024 •

edited

Loading

mwerle commented Oct 3, 2024 •

edited

Loading